Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix nginx template for custom s3 endpoints #3278

Open
wants to merge 5 commits into
base: preview
Choose a base branch
from

Conversation

dbtek
Copy link

@dbtek dbtek commented Dec 29, 2023

Custom s3 endpoints can be used via setting environment variables. However proxy is using local minio always.

@dbtek dbtek mentioned this pull request Dec 29, 2023
1 task
@aaryan610 aaryan610 requested a review from mguptahub December 29, 2023 12:20
@@ -30,7 +30,7 @@ http {
}

location /${BUCKET_NAME}/ {
proxy_pass http://plane-minio:9000/uploads/;
proxy_pass ${AWS_S3_ENDPOINT_URL}/${AWS_S3_BUCKET_NAME};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please append "/" at the end as

proxy_pass ${AWS_S3_ENDPOINT_URL}/${AWS_S3_BUCKET_NAME}/;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, my bad. Fixed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

I believe this is not the complete fix for custom S3 end point. As today if the files were uploaded to S3 (and not minio), the uploaded file url gets into the issue content. That means every time you see that file, it doesn’t go through proxy but directly from client to S3

if this has to go through proxy, application level code need to be modified as well.

Let me reconfirm with team and get back to you. @sriramveeraghanta @pablohashescobar

I will keep this PR on hold till we get clarity from team

Copy link
Author

@dbtek dbtek Dec 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirm this works with external minio and proxied asset urls. Application level modification would be required to use direct s3 urls other than current with proxy.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbtek, this change requires a significant change in the application we will keep this PR open till then once we have made the application we will merge this. Thank you for your contributions.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pablohashescobar is there any way to make external minio url work with docker deployment currently?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this proposed change I perfectly use external minio without any issues.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbtek Got it, but pr seems to be open for long time and this is blocker for anyone trying to use external minio

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@poudelprakash yeah I know. I built my own proxy image and advise anyone to the same as it seems that pr won't be merged anytime soon.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbtek Thanks to your pr and comments, I got it working now :)

@CLAassistant
Copy link

CLAassistant commented Feb 8, 2024

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants